Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/oidf 51 #19

Closed
wants to merge 17 commits into from
Closed

Feat/oidf 51 #19

wants to merge 17 commits into from

Conversation

jcmelati
Copy link
Collaborator

No description provided.

sanderPostma and others added 14 commits July 27, 2024 14:46
* Added Spring boot with status API

* chores: PR feedback

* Updated the server name in gradle module

* Added test for status endpoint and added README

* Enabled test run on gradle build

* Added Postgres and docker

* Added environment file

* Updated env variable

* Fixed merge conflicts

* Updated CI for GitHub secrets

* Added docker in GitHub action

* Added env variable for docker in GitHub action

* Removed Windows in GitHub action

* Added comment for removing Windows in CI

* Removed hardcoded path from run script

* fix: make admin server load env variables from root .env file (#10)

* chore: fix project name

* chore: extract ktor version

* fix: temporarily hardcode db credentials

* fix: import .env variables from file

* fix: adjust ci file to new docker compose dir

---------

Co-authored-by: sanderPostma <[email protected]>

---------

Co-authored-by: John Melati <[email protected]>
Co-authored-by: sanderPostma <[email protected]>
* Added Kermit logging

* Added logger class and added dependency in admin-server

* fix: adding env parameter for logging

* chores: removed logger env
fun createAccount(@RequestBody account: CreateAccountDTO) {
return accountService.create(account)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

fun findAll(): List<AccountDTO> {
return accountRepository.findAll().map { it.toDTO() }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

toolchain {
languageVersion = JavaLanguageVersion.of(21)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line


java {
toolchain {
languageVersion = JavaLanguageVersion.of(21)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will not work for multiplatform. It should be done in the jvm target. Ex:
kotlin { jvm { tasks { named<KotlinJvmCompile>("compileKotlinJvm") { dependsOn("fixOpenApiGeneratorIssue") compilerOptions { jvmTarget.set(JvmTarget.JVM_11) } } } } }


kotlin {
jvm{
withJava()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to mix Java code with Kotlin?

@@ -0,0 +1,6 @@
#Thu Jul 25 12:58:25 CEST 2024
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Unix carriage return. It should be \n and not \r\n.


expect object Persistence {
val accountRepository: AccountRepository
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

sealed class DatabaseConfig {
data class Postgres(val url: String, val username: String, val password: String) : DatabaseConfig()
data class Sqlite(val path: String) : DatabaseConfig()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

return when (config) {
is DatabaseConfig.Postgres -> platformSqlDriver.createPostgresDriver(config.url, config.username, config.password)
is DatabaseConfig.Sqlite -> platformSqlDriver.createSqliteDriver(config.path)
else -> throw IllegalArgumentException("Unsupported database config")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the exception message to a constant in a specific
file

SELECT * FROM account WHERE id = ?;

update:
UPDATE account SET username = ? WHERE id = ?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

val database = Database(driver)
accountRepository = AccountRepository(database)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

actual val accountRepository: AccountRepository

init {
val driver = PlatformSqlDriver().createPostgresDriver(System.getenv("DB_URL"), System.getenv("DB_USER"), System.getenv("DB_PASSWORD"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract a constant from strings and move them to a specific file

actual fun createSqliteDriver(path: String): SqlDriver {
throw UnsupportedOperationException("Sqlite is not supported on JVM")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add trailing new line

}

actual fun createSqliteDriver(path: String): SqlDriver {
throw UnsupportedOperationException("Sqlite is not supported on JVM")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract a constant from the exception message and move it a specific file

Copy link
Contributor

@zoemaas zoemaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is creating tables, something that is already implemented using Flyway. Will Flyway be removed from the project?

@@ -0,0 +1,29 @@
CREATE TABLE account (
Copy link
Contributor

@zoemaas zoemaas Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is table creation sqldelight's responsibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the "Fresh Schema" guide from here https://cashapp.github.io/sqldelight/2.0.2/jvm_postgresql/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you can combine them both, but we should follow the migration approach, ensuring a consistent DB over time

@jcmelati jcmelati closed this Jul 30, 2024
@jcmelati jcmelati reopened this Jul 30, 2024
@jcmelati jcmelati closed this Jul 30, 2024
@jcmelati jcmelati deleted the feat/oidf-51 branch July 30, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants